Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TCOPFLOW Python Bindings First Pass #131

Merged
merged 36 commits into from
May 21, 2024
Merged

TCOPFLOW Python Bindings First Pass #131

merged 36 commits into from
May 21, 2024

Conversation

cameronrutherford
Copy link
Contributor

@cameronrutherford cameronrutherford commented Mar 26, 2024 •
edited by jaelynlitz
Loading

Merge request type

  • New feature
  • Resolves bug
  • Documentation
  • Other

Relates to

  • OPFLOW
  • SOPFLOW
  • SCOPFLOW
  • TCOPFLOW
  • CMake build system
  • Spack configuration
  • Manual
  • Web docs
  • Other

This MR updates

  • Header files
  • Source code
  • CMake build system
  • Spack configuration
  • Web docs
  • Manual
  • Other - python bindings

Summary

Summarize the MR concisely

Linked Issue(s)

Link any relevant issues. If merging into the default branch, any directly addressed issues will be closed when merging into the default branch (develop).

Since GitHub is specific on language, make sure to include direct language if you want to have the issue closed:

  • Closes #issue
  • Resolves #issue

You can also reference issues, but this will not close the issue when the merge request is merged:

  • Partly related to #issue
  • #issue
  • Partially addresses #issue

@Jayapreethi Jayapreethi force-pushed the jaya/pytest branch 2 times, most recently from 225e564 to a62f321 Compare April 30, 2024 22:32
@Jayapreethi Jayapreethi self-assigned this Apr 30, 2024
@Jayapreethi
Copy link
Collaborator

Hi @cameronrutherford, this is ready for review. Please let me know about any further required changes .

@jaelynlitz jaelynlitz mentioned this pull request Apr 30, 2024
Closed
15 tasks
cameronrutherford
cameronrutherford commented May 1, 2024
View reviewed changes
Copy link
Contributor Author

@cameronrutherford cameronrutherford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Requested some changes but this is almost ready to merge

interfaces/python/CMakeLists.txt Show resolved Hide resolved
interfaces/python/example_tcopflow.py Show resolved Hide resolved
tests/interfaces/python/test_5_tcopflow.py Outdated Show resolved Hide resolved
tests/interfaces/python/test_5_tcopflow.py Outdated Show resolved Hide resolved
tests/interfaces/python/test_5_tcopflow.py Show resolved Hide resolved
cameronrutherford
cameronrutherford commented May 7, 2024
View reviewed changes
Copy link
Contributor Author

@cameronrutherford cameronrutherford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure as well to update the python interface markdown documentation @Jayapreethi

interfaces/python/example_tcopflow.py
tcopf.solve()
tcopf.print_solution(0)

# Delete instance before finalization (try, at least)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdym try, at least? Does this not work when running?

tests/interfaces/python/test_0_initialize.py Outdated
@@ -1,10 +1,12 @@
# test initializing exago only once
import exago
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import exago

Don't import exago before check_preconditions

tests/interfaces/python/test_5_tcopflow.py Outdated
@@ -0,0 +1,38 @@
import exago
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this import to after check_preconditions() is called.

Also, make sure to extend this fix to all of our other tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure to move this import.........

@cameronrutherford cameronrutherford mentioned this pull request May 7, 2024
Open
4 tasks
jaelynlitz
jaelynlitz requested changes May 8, 2024
View reviewed changes
Copy link
Contributor

@jaelynlitz jaelynlitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - please add the functions that you've ported in a new table for TCOPFLOW in this doc page then I think this is good to go

@cameronrutherford cameronrutherford changed the title Jaya/pytest TCOPFLOW Python Bindings First Pass May 8, 2024
@Jayapreethi
Copy link
Collaborator

Documented the tested functions of TCOPFLOW.

cameronrutherford
cameronrutherford commented May 14, 2024
View reviewed changes
Copy link
Contributor Author

@cameronrutherford cameronrutherford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor cleanup

tests/interfaces/python/test_0_initialize.py
import mpi4py.rc
mpi4py.rc.threads = False
from mpi4py import MPI # noqa
check_preconditions()
import exago # noqa


Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eliminate duplicate check_preconditions()

tests/interfaces/python/test_1_pflow.py Outdated
import exago # noqa


check_preconditions()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
check_preconditions()

tests/interfaces/python/test_2_opflow.py Outdated
import mpi4py.rc
mpi4py.rc.threads = False
from mpi4py import MPI # noqa
check_preconditions()
import exago # noqa

check_preconditions()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
check_preconditions()

tests/interfaces/python/test_3_scopflow.py Outdated
import mpi4py.rc
mpi4py.rc.threads = False
from mpi4py import MPI # noqa
check_preconditions()
import exago # noqa

check_preconditions()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
check_preconditions()

tests/interfaces/python/test_4_sopflow.py Outdated
import exago # noqa


check_preconditions()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
check_preconditions()

tests/interfaces/python/test_5_tcopflow.py Outdated
mpi4py.rc.threads = False
check_preconditions()

check_preconditions()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
check_preconditions()

tests/interfaces/python/test_9_finalize.py Outdated
from check_preconditions import check_preconditions
check_preconditions()


check_preconditions()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
check_preconditions()

tests/interfaces/python/test_5_tcopflow.py Outdated
@@ -0,0 +1,38 @@
import exago
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure to move this import.........

cameronrutherford
cameronrutherford commented May 16, 2024
View reviewed changes
Copy link
Contributor Author

@cameronrutherford cameronrutherford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final missed check_preconditions(), apart from that looks good to merge!

tests/interfaces/python/test_9_finalize.py Outdated
@@ -1,8 +1,7 @@
# test only run once things here
import pytest
import exago
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import exago

tests/interfaces/python/test_9_finalize.py
from check_preconditions import check_preconditions

check_preconditions()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
check_preconditions()
check_preconditions()
import exago

@cameronrutherford cameronrutherford mentioned this pull request May 17, 2024
Merged
@cameronrutherford
Copy link
Contributor Author

Spack ubuntu pipelines are failing... See #144

@jaelynlitz or @abhyshr with one final review I think this is good to merge! Thanks @Jayapreethi

@cameronrutherford cameronrutherford enabled auto-merge (squash) May 21, 2024 17:18
@cameronrutherford cameronrutherford merged commit 84fdbf2 into develop May 21, 2024
7 of 10 checks passed
@cameronrutherford cameronrutherford mentioned this pull request Jun 24, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaelynlitz jaelynlitz jaelynlitz requested changes

@abhyshr abhyshr abhyshr approved these changes

Assignees

@Jayapreethi Jayapreethi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cameronrutherford @Jayapreethi @abhyshr @jaelynlitz